-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix inconsistent typeSize calculation for TupleN vs recursive pair encodings
#24743
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
typeSize calculation for TupleN vs recursive pair encodings
cd3a120 to
dca3ea1
Compare
|
We could maybe use scala3/compiler/src/dotty/tools/dotc/core/TypeUtils.scala Lines 148 to 157 in f67a1a0
|
| val tpNorm = tp.tryNormalize | ||
| if tpNorm.exists then apply(n, tpNorm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| val tpNorm = tp.tryNormalize | |
| if tpNorm.exists then apply(n, tpNorm) | |
| val tpNorm = tp.normalized.normalizedTupleType | |
| if tpNorm ne tp then apply(n, tpNorm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normalizedTupleType does the opposite of what we want. We need to convert TupleN to *: equivalent. I found this function in TypeUtils.scala that does exactly this:
scala3/compiler/src/dotty/tools/dotc/core/TypeUtils.scala
Lines 122 to 126 in c82b623
| /** The `*:` equivalent of an instance of a Tuple class */ | |
| def toNestedPairs(using Context): Type = | |
| tupleElementTypes match | |
| case Some(types) => TypeOps.nestedPairs(types) | |
| case None => throw new AssertionError("not a tuple") |
Also note that tp.normalized on TupleN returns NoType.
Using this function, the code is cleaner and more readable:
def apply(n: Int, tp: Type): Int =
tp match {
case tp: AppliedType if defn.isTupleNType(tp) =>
foldOver(n + 1, tp.toNestedPairs)
// From here the following code is the same as the original
case tp: AppliedType =>
val tpNorm = tp.tryNormalize
if tpNorm.exists then apply(n, tpNorm)
else foldOver(n + 1, tp)
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to convert
TupleNto*:equivalent.
Why not do the opposite? TupleN needs less object allocations.
Also note that
tp.normalizedonTupleNreturnsNoType.
Maybe the following would work?
tp match {
case tp: AppliedType =>
val tpNorm = tp.tryNormalize
if tpNorm.exists then apply(n, tpNorm.normalizedTupleType)
else foldOver(n + 1, tp.normalizedTupleType)The above has the advantage that it always normalizes the type, even when it's a tuple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motivation of this issue is that the concatenation type of two tuples is made by a Match Type. When a TupleN and another tuple were to be concatenated, the result type uses the *: equivalent. The resulting typeSize is larger and created a lot of false positives in the algorithm of PR #24661
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but from my understanding, the requirement is simply that both TupleN and nested *: pairs be normalized to the same representation. Does it matter whether TupleN is normalized to nested pairs, or vice versa? If we normalize to TupleN, then both types in the unit tests would have size 1, wouldn't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess for your current implementation of match types termination checks, it's better if (1, 2) and (1, 2, 3) have sizes 2 and 3.
Previously, `typeSize` reported different values for standard `TupleN` types (e.g., `(A, B)`) compared to their equivalent recursive pair encodings (e.g., `A *: B *: EmptyTuple`). This discrepancy occurred because `TupleN` is a flat `AppliedType`, while the nested encoding forms a deeper tree structure. This patch modifies `TypeSizeAccumulator` to canonicalize `TupleN` types into their recursive `*:` representation before calculating the size. This ensures that the size metric is consistent regardless of whether the tuple is represented syntactically or structurally. This change is verified by a new unit test in `TypesTest`, which confirms that both `Tuple3[Int, Boolean, Double]` and its recursive equivalent `Int *: Boolean *: Double *: EmptyTuple` now yield identical `typeSize` values. Fixes scala#24730
dca3ea1 to
b290509
Compare
Currently,
typeSizereports inconsistent values for standard tuple types (e.g.,(A, B)) compared to their semantically equivalent recursive pair encodings (e.g.,A *: B *: EmptyTuple).This discrepancy arises because
TupleNis represented as a flatAppliedType, whereas the nested encoding forms a deeper tree structure. AstypeSizeis often used as a heuristic for complexity or optimization limits, this inconsistency can lead to divergent behavior in the compiler depending on how a tuple is represented syntactically.This PR modifies
TypeSizeAccumulatorto canonicalizeTupleNtypes into their recursive*:representation before calculating their size. This ensures that the size metric is consistent regardless of whether the tuple is represented as a flatAppliedTypeor a nested structural type.I have added a new unit test in
TypesTestthat assertsTuple3[Int, Boolean, Double]andInt *: Boolean *: Double *: EmptyTupleboth yield identicaltypeSizeequal to 3.Thanks to @mbovel for providing the unit test that effectively reproduces this issue and validates the fix.
Fixes #24730